Skip to content

Add missing "overrides" to realm-override config examples & tests#1511

Closed
RichardLiu2001 wants to merge 2 commits intoapache:mainfrom
RichardLiu2001:fix-realm-override-docs
Closed

Add missing "overrides" to realm-override config examples & tests#1511
RichardLiu2001 wants to merge 2 commits intoapache:mainfrom
RichardLiu2001:fix-realm-override-docs

Conversation

@RichardLiu2001
Copy link
Contributor

@RichardLiu2001 RichardLiu2001 commented May 2, 2025 •
edited
Loading

The existing examples of setting realm-override configurations are missing the "overrides" json key:

public interface FeaturesConfiguration {

  interface RealmOverrides {
    Map<String, String> overrides();
  }

So, instead of
polaris.features.realm-overrides."my-realm"."feature"=true
it should be
polaris.features.realm-overrides."my-realm".overrides."feature"=true

@github-project-automation github-project-automation bot moved this to PRs In Progress in Basic Kanban Board May 2, 2025
@RichardLiu2001 RichardLiu2001 changed the title add missing overrides Add missing "overrides" to config examples & tests May 2, 2025
@RichardLiu2001 RichardLiu2001 changed the title Add missing "overrides" to config examples & tests Add missing "overrides" to realm-override config examples & tests May 2, 2025
@RichardLiu2001 RichardLiu2001 marked this pull request as ready for review May 2, 2025 17:39
adutra
adutra previously approved these changes May 2, 2025
View reviewed changes
@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board May 2, 2025
@adutra adutra dismissed their stale review May 2, 2025 17:43

Thinking a bit more about where the issue is.

@adutra
Copy link
Contributor

adutra commented May 2, 2025 •
edited
Loading

@RichardLiu2001 sorry I approved too soon :-)

There are two issues here:

  1. You spotted a bug in the configmap generation where we were using polaris.config instead of polaris.features. I think it's legit to fix that.
  2. You spotted that the word .overrides. is missing in polaris.features.realm-overrides."my-realm".overrides."feature"=true – true, but I think the error is in FeaturesConfiguration.RealmOverrides: the RealmOverrides#overrides method should have been annotated with @WithParentName.

For 2, it's a regression introduced by b93e97b. @eric-maynard was your change intentional, that is, you want the word .overrides. to appear in the configuration, or is it OK to revert the changes to QuarkusFeaturesConfiguration?

eric-maynard
eric-maynard approved these changes May 3, 2025
View reviewed changes
Copy link
Contributor

@eric-maynard eric-maynard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch!

singhpk234
singhpk234 approved these changes May 3, 2025
View reviewed changes
Copy link
Contributor

@singhpk234 singhpk234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch !

@adutra
Copy link
Contributor

adutra commented May 3, 2025

I'm not sure this PR should be merged as is. If for nothing else, the default application.properties is not aligned with the Helm chart, for the reasons I mentioned in my previous comment.

@eric-maynard
Copy link
Contributor

eric-maynard commented May 3, 2025 •
edited
Loading

@adutra good point on (2) -- I would be okay changing it back to remove overrides... to be honest, I still find the post-Quarkus method of specifying a config's value really arcane. We went from e.g. featureConfiguration.SUPPORTED_CATALOG_STORAGE_TYPES to polaris.features.defaults."SUPPORTED_CATALOG_STORAGE_TYPES". Why is it not just featureConfiguration.X = Y?

@adutra
Copy link
Contributor

adutra commented May 3, 2025

@adutra good point on (2) -- I would be okay changing it back to remove overrides... to be honest, I still find the post-Quarkus method of specifying a config's value really arcane. We went from e.g. featureConfiguration.SUPPORTED_CATALOG_STORAGE_TYPES to polaris.features.defaults."SUPPORTED_CATALOG_STORAGE_TYPES". Why is it not just featureConfiguration.X = Y?

I'm all in to try and reduce the verbosity of the configuration options. But, I think it's also nice to always prefix our own options with "polaris." What I think we should try is to remove the "defaults." part. It should be possible imho by annotating the corresponding method with @WithParentName.

@adutra adutra mentioned this pull request May 12, 2025
Merged
gh-yzou
gh-yzou reviewed May 12, 2025
View reviewed changes
quarkus/defaults/src/main/resources/application.properties
# realm overrides
# polaris.features.realm-overrides."my-realm"."INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST"=true
# polaris.features.realm-overrides."my-realm"."SKIP_CREDENTIAL_SUBSCOPING_INDIRECTION"=true
# polaris.features.realm-overrides."my-realm".overrides."INITIALIZE_DEFAULT_CATALOG_FILEIO_FOR_TEST"=true
Copy link
Contributor

@gh-yzou gh-yzou May 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, never mind, this seems adding another key at the nest level, do we wan to add another key (which kind of changes how the config of overrides look like before), or revert the change before? cc @adutra @eric-maynard

Copy link
Contributor

@eric-maynard eric-maynard May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it works with @WithParentName, I think we should take that approach. From a behavior perspective, IMO we should try to revert back to the existing behavior. Ideally before 1.0.

Copy link
Contributor

@gh-yzou gh-yzou May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to fix to keep the old behavior, then the only doc update is "config" -> "features" change

@eric-maynard
Copy link
Contributor

eric-maynard commented May 12, 2025 •
edited
Loading

@adutra I'm hoping to fix this in #1572

... I think the error is in FeaturesConfiguration.RealmOverrides: the RealmOverrides#overrides method should have been annotated with @WithParentName ... is it OK to revert the changes to QuarkusFeaturesConfiguration?

Can you clarify whether FeaturesConfiguration.realmOverrides or QuarkusFeaturesConfiguration.QuarkusRealmOverrides.overrides needs the annotation? Per my understanding it's the latter but just want to confirm.

edit: from reading the docs, it seems like WithParentName should be on QuarkusFeaturesConfiguration.realmOverrides?

@eric-maynard eric-maynard mentioned this pull request May 12, 2025
Merged
@gh-yzou
Copy link
Contributor

gh-yzou commented May 12, 2025 •
edited
Loading

@eric-maynard In my other PR, i have a test to help testing he behavior https://github.com/apache/polaris/pull/1505/files#diff-d050121a0a07b1b4c8c7df231afbb927fe6cb04b2d4060a01fdbda96e9c568dfR45, you can probably add similar test in your fix to make sure the behavior is correct. However, based on my previous testing, it is QuarkusFeaturesConfiguration.QuarkusRealmOverrides.overrides needs the annotation.
i have one quick question, is there any fix needed for QuarkusBehaviorChangesConfiguration ?

@adutra
Copy link
Contributor

adutra commented May 12, 2025

Can you clarify whether FeaturesConfiguration.realmOverrides or QuarkusFeaturesConfiguration.QuarkusRealmOverrides.overrides needs the annotation? Per my understanding it's the latter but just want to confirm.

@eric-maynard the previous state before b93e97b would be to annotate only QuarkusFeaturesConfiguration.QuarkusRealmOverrides.overrides.

You can try to also annotate FeaturesConfiguration.realmOverrides but 1) that wouldn't match the previous state and 2) I'm not sure it would work because there is another map at the same level: FeaturesConfiguration.defaults.

@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board May 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eric-maynard eric-maynard eric-maynard approved these changes

@gh-yzou gh-yzou gh-yzou left review comments

@singhpk234 singhpk234 singhpk234 approved these changes

@adutra adutra adutra left review comments

@ashvina ashvina Awaiting requested review from ashvina

@dennishuo dennishuo Awaiting requested review from dennishuo

@dimas-b dimas-b Awaiting requested review from dimas-b

@jackye1995 jackye1995 Awaiting requested review from jackye1995

@jbonofre jbonofre Awaiting requested review from jbonofre

@vvcephei vvcephei Awaiting requested review from vvcephei

@collado-mike collado-mike Awaiting requested review from collado-mike

@snazy snazy Awaiting requested review from snazy

@RussellSpitzer RussellSpitzer Awaiting requested review from RussellSpitzer

@takidau takidau Awaiting requested review from takidau

@MonkeyCanCode MonkeyCanCode Awaiting requested review from MonkeyCanCode

@flyrain flyrain Awaiting requested review from flyrain

@ebyhr ebyhr Awaiting requested review from ebyhr

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@RichardLiu2001 @adutra @eric-maynard @gh-yzou @singhpk234